Config: accept a bare scalar for an array option in YAML syntax#5413
Config: accept a bare scalar for an array option in YAML syntax#5413arpitjain099 wants to merge 4 commits into
Conversation
In YAML config a single scalar value for an array option (for example retryable_response_codes: 503) reached Config.array_value as an Integer and was rejected with "array required but got 503", while the classic syntax retryable_response_codes 503 works because it arrives as the String "503" and gets split into ["503"]. Wrap a non-String, non-Array, non-Hash scalar into a one-element array so both syntaxes behave the same. Hash still raises as before. Fixes fluent#5149 Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
| "value_type: :integer w/o quote" => [[1,2,1], '1,2,1', {value_type: :integer}], | ||
| "bare integer (YAML scalar)" => [[503], 503, {}], | ||
| "bare integer w/ value_type" => [[503], 503, {value_type: :integer}], | ||
| "already an array is untouched" => [[503], [503], {}]) |
There was a problem hiding this comment.
nitpick: conversion rule was changed, so it might be better to add another test case such asa assert_raise(Fluent::ConfiguError.new("array required but got #{val.inspect}") for known types.
Could you afford to write such a test case?
Add a test case asserting Config::ARRAY_TYPE keeps raising Fluent::ConfigError with the "array required but got ..." message for a hash input, so the scalar-wrapping change does not accidentally accept a mapping under an array option. Covers a hash with and without value_type. Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
arpitjain099
force-pushed
the
chore/yaml-array-scalar
branch
from
1b8d35a to
5c314d9
Compare
|
Thanks for the review, good call. I've added a test case asserting that a Hash input still raises |
| else | ||
| [val] |
There was a problem hiding this comment.
One concern about the new else branch.
it wraps any object that isn't a String/Array/Hash, including Symbols, Time objects, or arbitrary objects that would previously have been rejected with ConfigError.
That defensive behavior is worth keeping for unexpected types.
Since the scalars Psych actually produces here are Integer/Float/true/false (nil is handled by the early return), how about limiting the wrapping to those types?
elsif val.is_a?(Numeric) || val == true || val == false
[val]
else
val # anything else still raises "array required" below
endThis fixes the reported issue while preserving the existing error for genuinely invalid values.
Limit the bare-scalar wrapping in Config.array_value to the scalar types Psych/YAML actually produces here (Numeric, true, false; nil is handled by the early return). Any other type such as a Symbol, Time, or Hash now falls through and still raises "array required", preserving the previous defensive rejection of unexpected values. Add tests covering bare float/true/false wrapping and that a Symbol and a Time still raise Fluent::ConfigError alongside the existing Hash case. Signed-off-by: arpitjain099 <arpitjain099@gmail.com>
|
Good catch, thanks. You're right that the broad |
|
The updated version looks good to me. One non-blocking suggestion, it would be nice to also have an end-to-end test in |
Watson1978
added
the
backport to v1.19
Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
|
Thanks @Watson1978, good idea. Added two tests to test/config/test_yaml_parser.rb that go through the real YAML pipeline: a config with retryable_response_codes: 503 on a match section, parsed with YamlParser and fed to a Configurable with an :array param (value_type: :integer). The bare scalar comes out as [503], and a [502, 503] sequence still works as before. Ran the file locally on Ruby 3.3, 33 tests green. |
3 participants
Which issue(s) this PR fixes:
Fixes #5149
What this PR does / why we need it:
In YAML config syntax, a single scalar value for an array option is rejected. For example
retryable_response_codes: 503fails witharray required but got 503, while the classic syntaxretryable_response_codes 503works fine. The difference is that the classic path delivers the value as the String"503", whichConfig.array_valuealready splits into["503"], whereas YAML hands it over as a bareIntegerthat falls straight through to the "array required" check.This wraps a non-String, non-Array, non-Hash scalar into a one-element array so both config syntaxes behave the same way. The wrapped value then flows through the existing
value_typecoercion, soretryable_response_codes: 503becomes[503]just likeretryable_response_codes: [503]does. A Hash still raises as before, so an accidental mapping under an array option is not silently accepted.I could not run the full suite locally (my Ruby is 2.6 and Fluentd needs 3.2), so I verified the logic by tracing the exact
array_valuepath and reproducing it in isolation; CI should confirm. I added test cases in test/config/test_types.rb covering a bare integer with and withoutvalue_type, and that an existing array is left untouched.Docs Changes:
None.
Release Note:
Fix a config error when a single scalar value is given to an array option in YAML config syntax (for example
retryable_response_codes: 503).